Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[14주차] 고태현 학습 PR 제출합니다 #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TaetaetaE01
Copy link
Member

인텔리제이와 DB 연동으로 많은 시간을 쏟아 지쳤습니다,, 일단 DB 연동이 안되어 코드만 구현했고,, 코드가 돌아가는지는 아직 확인 못하였습니다. 하지만 궁금한게 너무 많습니다

@TaetaetaE01 TaetaetaE01 changed the title [14주차] 고태현 학습 CODE 제출합니다 [14주차] 고태현 학습 PR 제출합니다 Jan 3, 2024
Copy link
Member

@stophwan stophwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다! DB 관련은 원만한 해결 바래요 ㅎㅎ
궁금한점이 있다면 Pr올릴때 작성하셔도 되고 디스코드의 질문게시판 많이 사용해주세요 ~~~

import java.util.List;

@RestController
@RequestMapping("api/board")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@RequestMapping("api/board")
@RequestMapping("/api/board")

Comment on lines +12 to +16
@Entity
@Getter
@Setter
@Builder
@NoArgsConstructor(access = AccessLevel.PROTECTED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지양해야하는 방식입니다. 저렇게 getter와 setter를 다 열어둔다면 필드를 Private으로 한 이유가 없습니다.

private String title;
private String content;

private Long member_id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

값자기 스네이크 케이스를 쓰신 이유가 있나요?
밑에서 이미 연관관계를 맺고 있는데 추가로 이걸 작성하신 이유가 궁금합니다!

Comment on lines +13 to +20
private Long id;
private String title;
private String content;
private Long memberId;


private String memberName;
private List<Reply> replyList;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private Long id;
private String title;
private String content;
private Long memberId;
private String memberName;
private List<Reply> replyList;
private Long id;
private String title;
private String content;
private Long memberId;
private String memberName;
private List<Reply> replyList;

@ManyToOne(fetch = FetchType.LAZY)
private Member member;

@OneToMany
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mappedBy 옵션이 없습니다!

Comment on lines +48 to +53
@Transactional(readOnly = true)
public Board checkBoard(Long id) {
Board board = boardRepository.findById(id)
.orElseThrow(() -> new NoSuchElementException("게시판 정보가 존재하지 않습니다."));
return board;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check라는것은 일반적으로는 boolean을 반환합니다 검증용 void 메서드일 수 있고요. check 이름의 메서드지만 Board를 반환하는 것은 메서드만 보았을때 정확히 무슨 메서드인지 확인하기 어려워보입니다!

Board board = checkBoard(id);

List<Reply> replyList = replyRepository.findByBoardId(id);
board.setReplyList(replyList);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setter 대신 의미있는 메서드 명을 사용하면 어떨까요?

Comment on lines +37 to +41
public void update(ReplyUpdateRequest replyUpdateRequest) {
this.id = replyUpdateRequest.getId();
this.member = replyUpdateRequest.getMember();
this.board = replyUpdateRequest.getBoard();
this.content = replyUpdateRequest.getContent();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흔히 댓글을 변경할때를 생각해면 댓글의 내용을 변경하는것이 일반적일꺼에요.
작성자나 댓글의 게시글이 갑자기 변경이 될 수는 없겠죠! 더더욱 id 식별자는 바뀌면 안되구요!

Comment on lines +15 to +22
public Reply toEntity() {
return Reply.builder()
.id(this.id)
.member(this.member)
.board(this.board)
.content(this.content)
.build();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Reply toEntity() {
return Reply.builder()
.id(this.id)
.member(this.member)
.board(this.board)
.content(this.content)
.build();
}
public Reply toEntity() {
return Reply.builder()
.id(id)
.member(member)
.board(board)
.content(content)
.build();
}

}

public static BoardResponse from(Board board) {
return new BoardResponse(board.getId(), board.getTitle(), board.getContent(), board.getMember().getId(), board.getMember().getName(), board.getReplyList());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

board.getMember().getId() 처럼 할경에 실행하시면 이상한 점을 느끼실 수 있을꺼에요! 직접 확인해보시고 문제점이 무엇인지 파악해보는 것을 추천드려요! 키워드는 LazyLoading 입니다.

@TaetaetaE01
Copy link
Member Author

고생하셨습니다! DB 관련은 원만한 해결 바래요 ㅎㅎ 궁금한점이 있다면 Pr올릴때 작성하셔도 되고 디스코드의 질문게시판 많이 사용해주세요 ~~~

pr 코멘트 주신대로 리펙토링 진행해보겠습니다! 감사합니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants